fix: fetch frontmatter imports: dependencies locally during add-wizard#17765
fix: fetch frontmatter imports: dependencies locally during add-wizard#17765
imports: dependencies locally during add-wizard#17765Conversation
…ompilation failure Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ted import errors Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
imports: dependencies locally during add-wizard
pkg/cli/remote_workflow.go
Outdated
| owner, repo := parts[0], parts[1] | ||
| ref := spec.Version | ||
| if ref == "" { | ||
| ref = "main" |
There was a problem hiding this comment.
Done in d561689. When spec.Version is empty, the function now calls getRepoDefaultBranch(spec.RepoSlug) to fetch the actual default branch from the GitHub API, falling back to "main" only if that call fails.
…hAndSaveRemoteFrontmatterImports Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added three more tests in d561689:
|
There was a problem hiding this comment.
Pull request overview
Fixes gh aw add-wizard failures for remote workflows that use YAML frontmatter imports: by fetching those imported markdown dependencies into the local repo before compilation, so the compiler can resolve them from disk instead of attempting remote downloads.
Changes:
- Added
fetchAndSaveRemoteFrontmatterImportsto download and write frontmatterimports:dependencies locally (with recursion support). - Wired the new fetch step into the remote add flow and stopped converting
imports:to workflowspec-pinned references duringadd-wizard. - Added unit tests intended to cover the new import-fetching behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
pkg/cli/remote_workflow.go |
Implements downloading/saving of frontmatter imports: dependencies for remote workflows. |
pkg/cli/add_command.go |
Calls the new import-fetch function during remote add flow and keeps imports: as relative paths. |
pkg/cli/remote_workflow_test.go |
Adds tests for the new import-fetching function (currently mostly no-op/skip-path coverage). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/cli/remote_workflow.go
Outdated
| // Recursively fetch any nested imports declared in this file | ||
| if err := fetchAndSaveRemoteFrontmatterImports(string(importContent), spec, targetDir, verbose, force, tracker); err != nil && verbose { | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to fetch nested imports from %s: %v", filePath, err))) | ||
| } |
There was a problem hiding this comment.
Recursive call uses the same spec.WorkflowPath-derived workflowBaseDir for nested imports, so relative imports inside an imported file will be resolved relative to the original workflow, not the imported file’s directory. Also, seen is scoped per call, so cyclic imports can recurse indefinitely. Pass the current file path/base dir into recursion and share a single visited set across the entire traversal to prevent cycles and resolve nested relative paths correctly.
pkg/cli/remote_workflow.go
Outdated
| // Download from the source repository | ||
| importContent, err := parser.DownloadFileFromGitHub(owner, repo, remoteFilePath, ref) | ||
| if err != nil { | ||
| if verbose { | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to fetch import %s: %v", filePath, err))) | ||
| } | ||
| continue | ||
| } |
There was a problem hiding this comment.
Errors while downloading/writing imports are swallowed via continue, and the function ultimately returns nil even if no imports were fetched. This makes the error return value misleading and prevents callers from reacting to partial failures. Consider collecting failures and returning an aggregated error (or change the signature/behavior to explicitly indicate “best-effort” and surface which imports failed).
pkg/cli/remote_workflow_test.go
Outdated
| // Content with two imports that differ only in their #section fragment. | ||
| // Both should resolve to the same file (deduplication by file path). | ||
| content := `--- | ||
| engine: copilot | ||
| imports: | ||
| - github/gh-aw/.github/workflows/shared/reporting.md@abc123 | ||
| - github/gh-aw/.github/workflows/shared/reporting.md@abc123 | ||
| --- |
There was a problem hiding this comment.
This test claims to verify section-fragment stripping/deduplication, but the imports: entries don’t include any #section fragments—both lines are identical workflowspecs. Update the fixture to include .../reporting.md#SectionA and .../reporting.md#SectionB (and avoid workflowspec format if you actually want to exercise the fetch/dedup path).
pkg/cli/remote_workflow_test.go
Outdated
| // TestFetchAndSaveRemoteFrontmatterImports_FileTracking verifies that files saved by | ||
| // the function are properly tracked (created vs modified). | ||
| func TestFetchAndSaveRemoteFrontmatterImports_FileTracking(t *testing.T) { | ||
| // Use a temp dir that acts as the workflows directory. | ||
| tmpDir := t.TempDir() | ||
|
|
||
| // Pre-create one file so we can test the "modified" branch. | ||
| sharedDir := filepath.Join(tmpDir, "shared") | ||
| require.NoError(t, os.MkdirAll(sharedDir, 0755)) | ||
| existingFile := filepath.Join(sharedDir, "existing.md") | ||
| require.NoError(t, os.WriteFile(existingFile, []byte("old content"), 0600)) | ||
|
|
||
| // Build a minimal FileTracker without calling NewFileTracker (which requires a real | ||
| // git repository). We only need the tracking lists populated. | ||
| tracker := &FileTracker{ | ||
| OriginalContent: make(map[string][]byte), | ||
| gitRoot: tmpDir, | ||
| } | ||
|
|
||
| // A workflow with no imports should leave the tracker empty. | ||
| content := `--- | ||
| engine: copilot | ||
| --- | ||
|
|
||
| # No imports | ||
| ` | ||
| spec := &WorkflowSpec{ | ||
| RepoSpec: RepoSpec{ | ||
| RepoSlug: "github/gh-aw", | ||
| Version: "v1.0.0", | ||
| }, | ||
| WorkflowPath: ".github/workflows/test.md", | ||
| } | ||
|
|
||
| err := fetchAndSaveRemoteFrontmatterImports(content, spec, tmpDir, false, false, tracker) | ||
| require.NoError(t, err) | ||
| assert.Empty(t, tracker.CreatedFiles, "no files should be created when there are no imports") | ||
| assert.Empty(t, tracker.ModifiedFiles, "no files should be modified when there are no imports") | ||
| } |
There was a problem hiding this comment.
Despite the name/comment, this test never exercises the created/modified tracking branches: it uses content with no imports and only asserts the tracker remains empty. Either add an import and stub the downloader so the function writes a new file (created) and overwrites an existing file (modified), or rename the test to reflect that it only covers the no-imports no-op case.
pkg/cli/remote_workflow_test.go
Outdated
| // Use a content with a relative import that would resolve to the pre-created file. | ||
| // But since the import path is workflowspec-format, it's skipped regardless – | ||
| // this test verifies no modification is done to the pre-created file. | ||
| content := `--- | ||
| engine: copilot | ||
| imports: | ||
| - github/gh-aw/.github/workflows/shared/ci-data-analysis.md@abc123 |
There was a problem hiding this comment.
This test is named/worded as “skip existing without force”, but it uses a workflowspec-format import which is skipped unconditionally by isWorkflowSpecFormat, so it doesn’t validate the os.Stat/force logic for relative imports. Use a relative import path (and stub the download) to confirm the existing file is preserved when force=false.
| // Use a content with a relative import that would resolve to the pre-created file. | |
| // But since the import path is workflowspec-format, it's skipped regardless – | |
| // this test verifies no modification is done to the pre-created file. | |
| content := `--- | |
| engine: copilot | |
| imports: | |
| - github/gh-aw/.github/workflows/shared/ci-data-analysis.md@abc123 | |
| // Use content with a relative import that resolves to the pre-created file. | |
| // Because the file already exists on disk and force=false, the implementation | |
| // should detect the existing file (via os.Stat) and skip downloading or | |
| // overwriting it, preserving its original content. | |
| content := `--- | |
| engine: copilot | |
| imports: | |
| - shared/ci-data-analysis.md |
pkg/cli/remote_workflow.go
Outdated
| targetPath := filepath.Join(targetDir, filepath.FromSlash(filePath)) | ||
|
|
||
| // Create the target directory if needed | ||
| if err := os.MkdirAll(filepath.Dir(targetPath), 0755); err != nil { | ||
| if verbose { | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to create directory for import %s: %v", filePath, err))) |
There was a problem hiding this comment.
targetPath := filepath.Join(targetDir, filepath.FromSlash(filePath)) is vulnerable to absolute-path and path traversal writes. If filePath starts with "/", Join will ignore targetDir and write to an arbitrary absolute location; .. segments can also escape targetDir. Sanitize/normalize filePath (strip leading "/", reject/clean ..), and verify the final targetPath stays within targetDir (e.g., via filepath.Rel boundary check like pkg/cli/poutine.go:417-444).
| targetPath := filepath.Join(targetDir, filepath.FromSlash(filePath)) | |
| // Create the target directory if needed | |
| if err := os.MkdirAll(filepath.Dir(targetPath), 0755); err != nil { | |
| if verbose { | |
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to create directory for import %s: %v", filePath, err))) | |
| relPath := filepath.Clean(filepath.FromSlash(filePath)) | |
| // Strip any leading path separators so an absolute path cannot escape targetDir. | |
| relPath = strings.TrimLeft(relPath, string(filepath.Separator)) | |
| // Reject empty or parent-directory-relative paths. | |
| if relPath == "" || relPath == "." || relPath == ".." || strings.HasPrefix(relPath, ".."+string(filepath.Separator)) { | |
| if verbose { | |
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Refusing to write import with unsafe path %q", filePath))) | |
| } | |
| continue | |
| } | |
| targetPath := filepath.Join(targetDir, relPath) | |
| // Ensure the final targetPath is still within targetDir. | |
| if rel, err := filepath.Rel(targetDir, targetPath); err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { | |
| if verbose { | |
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Refusing to write import outside target directory: %q", filePath))) | |
| } | |
| continue | |
| } | |
| // Create the target directory if needed | |
| if err := os.MkdirAll(filepath.Dir(targetPath), 0755); err != nil { | |
| if verbose { | |
| fmt.Fprintln(os.Stderr, os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to create directory for import %s: %v", filePath, err))) |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot apply comments, add guardrails against recursive imports. |
…improve tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in e0308c9. Applied all reviewer comments and added recursive import guardrails:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
gh aw add-wizardon any workflow that uses theimports:frontmatter field (e.g.ci-coach) always failed: the add flow converted relative import paths to remote workflowspecs (github/gh-aw/.github/workflows/shared/ci-data-analysis.md@<sha>), the compiler tried to download them at compile time, the download failed, and because the lock file was pre-tracked but never written,git addexited 128.Changes
pkg/cli/remote_workflow.go— NewfetchAndSaveRemoteFrontmatterImports(public entry point) delegates to an internalfetchFrontmatterImportsRecursivehelper. The helper:seenmap (keyed by fully-resolved remote path) so import cycles (A→B→A) and duplicate imports across any nesting depth are broken without infinite recursion.currentBaseDir(changes per recursion level, set to the imported file's directory) andoriginalBaseDir(constant, the top-level workflow's directory) so relative paths inside nested imports resolve correctly and map to the right local path undertargetDir.os.Statbefore the network download, so already-present files skip the download entirely whenforce=false.remoteFilePathvalues that start with..(escaping the repository root), deriveslocalRelPathby strippingoriginalBaseDir, and applies afilepath.Relbelt-and-suspenders boundary check to ensuretargetPathstays withintargetDir.getRepoDefaultBranchto query the actual default branch of the source repository (falls back to"main"if the API call fails) and persists the resolved ref onspec.Version.pkg/cli/add_command.go— CallsfetchAndSaveRemoteFrontmatterImportsimmediately afterfetchAndSaveRemoteIncludesin the remote-workflow path. Removes the subsequentprocessImportsWithWorkflowSpeccall so import paths remain as relative references; the compiler then resolves them from disk (the files are now present locally) rather than attempting remote downloads.pkg/cli/remote_workflow_test.go— Unit tests for the new function covering: no imports, local workflow (emptyRepoSlug), already-pinned workflowspec imports skipped, no-imports no-op tracker, section-fragment deduplication (shared/reporting.md#SectionA+#SectionBdeduplicated without a second download), skip-existing-without-force (relative import, pre-existing file, no network needed because stat check precedes download), path traversal rejection (../etc/passwdwith workflow at repo root triggers the explicit..rejection before any network call), and invalidRepoSlugearly-exit.The resulting on-disk
ci-coach.mdretainsimports: - shared/ci-data-analysis.md(relative), which is also what the compiled lock file needs for its{{#runtime-import .github/workflows/shared/ci-data-analysis.md}}macros to resolve at runtime.Original prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.